Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show overall duration of videos in playlist #6045

Merged
merged 1 commit into from
Mar 29, 2024

Conversation

bg172
Copy link

@bg172 bg172 commented Apr 10, 2021

earlier only overall amount of videos was shown in playlist (in both searched online playlist and local playlist). Now overall duration is shown there too - as formatted by existing Localization.concatenateStrings() and Localization.getDurationString().

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

earlier only overall amount of videos was shown. Now overall duration is shown there too - as formatted by existing Localization.concatenateStrings() and Localization.getDurationString().

APK testing

see https://github.com/TeamNewPipe/NewPipe/pull/6045/checks (artefacts) for the uptodate APK as soon as built by github - tested OK on Xiaomi Mi 9 SE

Due diligence

@bg172 bg172 marked this pull request as draft April 10, 2021 21:57
@bg172 bg172 marked this pull request as ready for review April 10, 2021 22:14
Copy link
Collaborator

@XiangRongLin XiangRongLin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work like this. The remote playlist are paginated, meaning you only ever load a fraction of longer playlist. The playlist also can't be preloaded, because it could potentially be an infinite amount of videos (YouTube Mix)

It also looks like as if the change will break the existing counter for long playlist, for the same reason.

@XiangRongLin
Copy link
Collaborator

This would need to be done in the extractor, in order to be done properly. And it would only work if the services provide that specific information for us to parse

@bg172
Copy link
Author

bg172 commented Apr 11, 2021

Hi @XiangRongLin, thank you very much for reviewing. Now reworked to show overall duration only for local lists - they are not so tricky I hope. Can you give an example-link of an infinite list by the way?

@bg172 bg172 requested a review from XiangRongLin April 11, 2021 08:12
@XiangRongLin
Copy link
Collaborator

Can you give an example-link of an infinite list by the way?

https://www.youtube.com/watch?v=FfAjtZgLkPQ&list=RDFfAjtZgLkPQ

Its basically youtube.com/watch?v={videoID}&list=RD{videoID}

@bg172
Copy link
Author

bg172 commented Apr 11, 2021

so now improved enough, tried with that infinite list. Next thing would be to extend the service to deliver the duration, but this service is not in our hands, right?:)

@XiangRongLin
Copy link
Collaborator

The service is in our hands https://github.com/TeamNewPipe/NewPipeExtractor

And like i said before, it needs to be fixed there. From a quick glance the duration would increase as more items are loaded.
Imagine you are a user and experience that. Would you not be confused?

@bg172
Copy link
Author

bg172 commented Apr 11, 2021

I am a user, and i am already satisfied with this - i did this implementation for me in the first line;)
Most of playlists i see are under 100 items and this seems to be the threshold of the service currently to provide items, i.e. in many cases the duration will be correct, and the longer it is the less important to know its value exactly;)
To not confuse users there is this "+" in the end of duration string like 12:20:23+ until exact entire duration is known, meaning it is longer than 12h20m23s.
With your info that the service is editable, i consider my PR as kind of an incremental improvement, but not sure when i can make the next incremental step. And concerning its need: large durations e.g. thousands of videos would take also more space in the UI and might cost essential performance, not obvious anybody wants it.
What do you think?

@triallax
Copy link
Contributor

I think this could be made as an option disabled by default, with a note that for larger playlists, it might not display the actual duration immediately.

@bg172
Copy link
Author

bg172 commented May 1, 2021

now made it configurable

@bg172
Copy link
Author

bg172 commented May 1, 2021

This would need to be done in the extractor, in order to be done properly. And it would only work if the services provide that specific information for us to parse

Hi @XiangRongLin , if I do this via the extractor I have to return the overall duration in PlaylistInfo which is called per current design only once - when opening the view for a playlist. Because the online-service (e.g. youtube) does not provide playlist-duration it is needed to loop through all items, sum up the duration and provide it via something like PlaylistInfo.getStreamDuration() of extractor - for multipage playlists (>100 videos) it is needed to load all pages which takes too long (about 12s to show a playlist with 1615 videos I had tried - all this time the list itself appears to be loading and no items of it is shown to the user) which makes user experience too bad.
The alternative which I implemented - summing up only for the items which are currently shown and considering additional items incrementally as soon as they are added - does not fit current design of extractor usage: the callback adding next page items handleNextItems(final ListExtractor.InfoItemsPage result) cannot read the original PlaylistInfo which would have new getter getStreamDuration() i.e. does not allow to update the duration incrementally.

So either we implement outside of extractor showing only partial duration in case of long playlist but having good performance, or we implement inside of extractor but kill the performance and show the exact duration which nobody needs in long playlists:)

What do you think, am I missing something?

@XiangRongLin
Copy link
Collaborator

That is unfortunate that youtube does not provide that information. Since the services (youtube) themself do not provide that information, it would then also not belong inside the extractor.

So either we implement outside of extractor showing only partial duration in case of long playlist but having good performance

I don't personally see much use in this solution, but I also don't like to decide about which features to add and which not. These discussions are best made in an issue and not the PR itself.
@Stypox your thoughts on this, i'm slightly against this.

@bg172
Copy link
Author

bg172 commented May 14, 2021

"without opening the playlist" is a good point, let me try.

Concerning units: I made it already without units, it looks like this now: 01:35:17 in case complete or in case incomplete 12:08:09+,
but 12:08:09 (∞) is also ok if you confirm, @SameenAhnaf.

About "first 50 or 100 videos": it works already like this out-of-the-box just because this amount of videos the extrator delivers.

@bg172
Copy link
Author

bg172 commented May 15, 2021

hi @SameenAhnaf ,
I checked about "without opening the playlist", and unfortunately cannot do because e.g. youtube provides info/duration of only 2 first videos on the search-results page with playlists - showing overall duration of just two first videos appears meaningless to me. Only when one opens the playlist, a meaningful amount of info/durations of videos becomes available. Might be possible with a larger redesign perhaps by sending more requests than the user directly triggers, but I cannot do it, sorry.

Showing duration for a channel (when one is open on the screen) I would consider as a new PullRequest, and seems to be possible as I understand the corresponding youtube page for that. But on my phone-screen there seems to be no space where to add duration - even the number of videos is not shown, see
Screenshot 2021-05-15 023623channel
Were your screenshots done on some larger device like a tablet? Please suggest.

And some good news: on your screenshot with infinite number of videos no duration is shown I bet because it was not enabled in Settings as requested by @mhmdanas:), I now updated the link to the latest APK, sorry for confusion, can you please check again.
Screenshot 2021-05-15 020735settings
Screenshot 2021-05-15 021000infList
Screenshot 2021-05-15 021157smallList
image

Cool that you've found my feature useful, looking forward to hear from you.

@bg172
Copy link
Author

bg172 commented May 15, 2021

It seems like that we will have to keep the information beside the channel name as you can see for grid view.
IMG_20210515_105019

This screenshot is a showing search-results page with playlists from youtube, for which youtube does not provide durations - not sure if possible to implement without youtube support but can you create a new feature request?

Thanks once again. But I wonder why this setting should be optional.
Maybe, a setting like "Show duration of next unwatched videos" with options "None/All (Watched+Unwatched)/5/10/25/50/100 videos" could be added. This setting is required as none of us won't always have the time to stream hundreds of videos at a sitting.

If "All" is selected for an infinite playlist and duration of first 100 videos is 19:09:08, duration could be shown as "19:09:08+". If a certain value is set (5/10/25/50/100), no '+' icon needs to be shown for infinite playlists. If certain value (5/10/25/50/100) is set for this setting, "Watch history" is disabled, duration of first (5/10/25/50/100) videos will be shown.

Yeah, the dependency on watch-history would be a new advanced feature building upon this PR. And it would depend on the sort-by criteria too, which I was dreaming of as next implementation. Until that this setting is too much, therefore I will remove it now.

@triallax triallax added feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface labels Jul 20, 2021
@litetex litetex added the discussion This needs to be discussed before anything is done label Mar 15, 2022
@Akv2021
Copy link

Akv2021 commented Jul 17, 2022

Any chances of merging this feature ?
I think NewPipe can provide an "experimental features" section which enables features like above which might not work for all users but are useful for the smaller audience nonetheless.

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this nice addition. Code looks almost good, and is also really simple. I think this PR should be merged, but we should not proceed further imo (e.g. by implementing it in the extractor), because we would soon get into niche territory.

By the way, sorry, I completely missed this PR for some reason

headerBinding.playlistStreamCount.setText(Localization
.localizeStreamCount(activity, count));
final long videoCount = itemsList.size();
final long playlistOverallDurationSeconds = itemsList.stream().mapToLong(x ->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would first filter by PlaylistStreamEntry.class::isInstance, then map with PlaylistStreamEntry.class::cast, then map to PlaylistStreamEntry::getStreamEntity, finally mapToLong StreamEntryOrWhatever::getDuration and sum.

@bg172
Copy link
Author

bg172 commented Jul 19, 2022

Hi @Stypox, @Akv2021, thank you for your comments and attention to this pull-request!
Meanwhile I detected there is already a similar feature in NewPipe available (v0.23.1), see videos attached - e.g. in a found list or a favorite list you can see the full duration of the list and remaining list-time over the list if you switch to a next video. Let me know what do you think:)

Screenrecorder-2022-07-19-19-29-17-300.mp4
Screenrecorder-2022-07-19-19-30-53-869.mp4

@Stypox
Copy link
Member

Stypox commented Jul 19, 2022

I think this PR would be useful anyway, even if a similar feature exists in another place

@opusforlife2 opusforlife2 added the playlist Anything to do with playlists in the app label Oct 24, 2022
@TobiGr TobiGr force-pushed the showOverallDurationInPlaylist branch from 6d02491 to 2b684ad Compare July 17, 2023 19:26
@TobiGr TobiGr requested a review from Stypox July 17, 2023 19:28
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

0.0% 0.0% Coverage
0.0% 0.0% Duplication

earlier only overall amount of videos was shown. Now overall duration is shown there too - as formatted by existing Localization.concatenateStrings() and Localization.getDurationString().

show all videos OverallDuration in local Playlist too

refactor to make implementation in LocalPlaylistFragment and PlaylistFragment more obviously similar

unfortunately could not refactor upto BaseLocalListFragment

revert the changes for online Playlists

because they are paginated and may be infinite i.e. correct count may come only from the service->extractor chain which unfortunately does not give overall duration yet

next try to improve user-experience with online Playlist

just show that duration is longer (">") than the calculated value in case there is more page(s)

even more improve user-experience for online Playlist

by adding the duration of next items as soon as they are made visible

make showing of playlists duration configurable, disabled by default

adjusted duration to be handled as long because it comes as long from extractor

no idea why I handled it as int earlier

Revert "make showing of playlists duration configurable, disabled by default", refactor

This reverts commit bc1ba17.

Fix rebase

Apply review

Rename video -> stream

Remove unused settings keys
@Stypox Stypox force-pushed the showOverallDurationInPlaylist branch from 72f34ee to bef5907 Compare March 29, 2024 13:11
@github-actions github-actions bot added the size/medium PRs with less than 250 changed lines label Mar 29, 2024
Copy link

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you to everyone involved! I rebased and squashed, and removed some leftover strings that were used in the settings. I also tested both local and remote playlists, adding and removing items, and everything worked.

@Stypox Stypox enabled auto-merge March 29, 2024 13:25
@Stypox Stypox merged commit 3c0a200 into TeamNewPipe:dev Mar 29, 2024
6 checks passed
@AudricV AudricV changed the title show overall duration of videos in playlist Show overall duration of videos in playlist Mar 29, 2024
@Stypox Stypox mentioned this pull request Apr 1, 2024
8 tasks
@opusforlife2
Copy link
Collaborator

It feels like the current UI is a bit non-intuitive. Maybe there should be a prefix before it, like "Total duration: 45:03" or something.

@bg172
Copy link
Author

bg172 commented Apr 4, 2024

It feels like the current UI is a bit non-intuitive. Maybe there should be a prefix before it, like "Total duration: 45:03" or something.

A nice suggestion! I don't know how to update this pull-request here probably because it is marked as closed, but I have an idea how to fulfill this request - see dev...bg172:NewPipe:showOverallDurationInPlaylist . Maybe somebody can take it over in a meaningful way?

image image

@opusforlife2
Copy link
Collaborator

@Stypox Does this need a new PR?

@Stypox
Copy link
Member

Stypox commented Apr 4, 2024

Yes, this should go in a new PR, but I guess it will be a quite simple change, so it can be merged in 0.27.0

@opusforlife2
Copy link
Collaborator

@bg172 ^ ^_^

@bg172
Copy link
Author

bg172 commented Apr 6, 2024

Yes, this should go in a new PR, but I guess it will be a quite simple change, so it can be merged in 0.27.0

#10952

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion This needs to be discussed before anything is done feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface playlist Anything to do with playlists in the app size/medium PRs with less than 250 changed lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants